-
Notifications
You must be signed in to change notification settings - Fork 46.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Follow-up to initial Trusted Types support #16795
Conversation
// TrustedURLs are deprecated and will be removed soon: https://github.com/WICG/trusted-types/pull/204 | ||
const isURL = trustedTypes.isURL ? trustedTypes.isURL : value => false; | ||
toStringOrTrustedType = value => { | ||
if (typeof value === 'string') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess numbers should also go here.
return '' + value; | ||
} | ||
export let toStringOrTrustedType: any => string | TrustedValue = toString; | ||
if (enableTrustedTypesIntegration && typeof trustedTypes !== 'undefined') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like it would make this function not inlinable if the flag is on or dynamic which isn’t great because it used to be a simple ’’ +
that gets inlined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That’s actually what got me looking at it in the first place — it wasn’t getting inlined with a dynamic flag in FB bundle. Lemme think about this more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the flag is on, it's already not being inlined, even before this PR. Probably because the body is bigger. Since we're doing a function call either way, I think a smaller function (as in this PR) would be better than a larger one. It's also a single call (in this PR) rather than two calls (as in before).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to get this in because it's not being inlined either way with the flag on. If we want to fix this, we'll have to inline it manually at concrete callsites.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we toString in user space instead of letting the browser do it again?
Seems like we only need it in some IE legacy thing and/or for the javascript:
URL check so we should only ever do this at all in those cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's for IE9 I think? We could feature-test it I guess.
return <div dangerouslySetInnerHTML={{__html: this.state.inner}} />; | ||
} | ||
it('should not stringify trusted values for dangerouslySetInnerHTML', () => { | ||
const ttObject1 = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would move this to beforeEach as it's being used in a few places.
toStringOrTrustedType = value => { | ||
if ( | ||
typeof value === 'object' && | ||
(isHTML(value) || isScript(value) || isScriptURL(value) || isURL(value)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fails with Illegal invocation error (is* functions need to be bound to the factory i.e. window.[tT]rustedTypes
.
This addresses a few things I've noticed since merging #16157.
toStringOrTrustedType
.toStringOrTrustedType
totoString
directly, and then adds the TT checks on top when the flag is on. I've inlinedisTrustedTypesValue
into that branch since it wasn't useful by itself.isHTML
and friends early to avoid property access hit in the hot path. I'm assumingtrustedTypes
methods don't care aboutthis
values. This might make mocking a bit more difficult but IMO this makes sense because we decide early on whether to use them or not. As opposed to TT support potentially "turning on" later in the app. This does mean, however, that TT needs to be initialized before ReactDOM to be acknowledged.toStringOrTrustedType
call with'' + value
now fails a particular test so we're covering them all.setInnerHTML
a bit to remove the duplicateinnerHTML
assignment which is easy to accidentally mess up. The special path now has an early return.